Reviewed all commit messages.
Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @mojganii)

ios/MullvadREST/Transport/TransportProvider.swift line 94 at r1 (raw file):

    /// Returns a randomly selected shadowsocks configuration.
    private func makeBridgeConfiguration() throws -> ShadowsocksConfiguration {

We shouldn't be renaming this.
The term bridge has some implication in networks, and a shadowsocks connection is not necessarily used to create bridged networks.
Please revert this to the previous name.

ios/MullvadREST/Transport/TransportStrategy.swift line 19 at r1 (raw file):

        /// Connecting via local Shadowsocks proxy
        case bridge

We don't need this case, since we're using shadowsocks to connect via bridges.
We can just reuse shadowsocks with a local configuration instead of a user defined one.

ios/MullvadREST/Transport/TransportStrategy.swift line 45 at r1 (raw file):

        self.userDefaults = userDefaults
        self.dataSource = datasource
        self.lastReachableConfigurationId = UUID(

There are 2 problems with this

• UUID(uuidString: "") always returns nil
  So if there is no value for LastReachableConfigurationCacheKey it will fail anyway, and we don't need to construct an invalid UUID for this
• This class shouldn't select itself how to get a lastReachableConfigurationId, it should be dependency injected to it.

That guarantees that the class is using the parameters provided to it, and will make it easier to debug if something goes wrong.

ios/MullvadREST/Transport/TransportStrategy.swift line 86 at r1 (raw file):

    /// Returning `Direct` if there is no enabled configuration
    private func next() -> PersistentAccessMethod {
        let direct = dataSource.accessMethods.first(where: { $0.kind == .direct })!

We do this a lot, it would probably be useful to add that as an extension in AccessMethodRepositoryDataSource to query it directly.

public extension AccessMethodRepositoryDataSource {
    var directAccess: PersistentAccessMethod {
        accessMethods.first(where: {$0.kind == .direct})!
    }
}

Code quote:

dataSource.accessMethods.first(where: { $0.kind == .direct })!

Reviewable status: 0 of 17 files reviewed, 10 unresolved discussions (waiting on @mojganii)

ios/MullvadRESTTests/TransportStrategyTests.swift line 10 at r1 (raw file):

@testable import MullvadREST
import MullvadSettings

import @testable MullvadSettings

ios/MullvadRESTTests/TransportStrategyTests.swift line 33 at r1 (raw file):

    }

    func testContinuesToUseDirectWhenNoOneIsEnabled() {

How about func testDefaultStrategyIsDirectWhenAllMethodsAreDisabled ?

Code quote:

testContinuesToUseDirectWhenNoOneIsEnabled

ios/MullvadRESTTests/TransportStrategyTests.swift line 58 at r1 (raw file):

    }

    func testContinuesToUseBridgeWhenJustOneIsEnabled() {

This test name is misleading IMHO
What we are testing here is that if one method is enabled, it's always selected.
The fact that it's bridges or something else doesn't change that.

I would probably rename this testSameMethodIsReusedIfEverythingElseIsDisabled

Code quote:

testContinuesToUseBridgeWhenJustOneIsEnabled

ios/MullvadRESTTests/TransportStrategyTests.swift line 83 at r1 (raw file):

    }

    func testContinuesToUseDirectWhenItReachesEnd() {

Same as above, this is misleading. direct can be disabled, so what we are really testing here is that we loop over all the enabled methods infinitely when one fails.

To this effect, I propopse that we do the following for this test

• Disable direct
• Add 2 more methods , one shadowSocks, one socks5
• We loop over all the strategies available twice (to make sure we did loop) and we assert the current strategy after each failure is different than the previous one. (We don't need to check the strategy itself, since the next test does that)

It's a big test, but it's important that we get this right. The current version of the test doesn't really test much, and wouldn't detect if the selection was broken to only ever return direct

Also we could probably rename this func testLoopsFromTheStartAfterTryingAllEnabledStrategies

ios/MullvadRESTTests/TransportStrategyTests.swift line 152 at r1 (raw file):

        XCTAssertEqual(
            transportStrategy.connectionTransport(),
            .shadowsocks(configuration: ShadowsocksConfiguration(

We can (and should, for readability) reuse that object across the test by declaring it as class member
Same for the direct and bridges methods, it would make the tests easier to read.

ios/MullvadRESTTests/TransportStrategyTests.swift line 171 at r1 (raw file):

                .cipher == config2.cipher && config1.password == config2.password
        case let (.socks5(config1), .socks5(config2)):
            return config1.address.rawValue == config2.address.rawValue && config1.port == config2.port

AnyIPAddress implements Equatable
No need for the rawValue checks here

Reviewable status: 0 of 17 files reviewed, 10 unresolved discussions (waiting on @buggmagnet)

ios/MullvadREST/Transport/TransportProvider.swift line 94 at r1 (raw file):

I took the part are related to loading/creating local shadow-socks proxy into separate object.while we added custom shadow-socks and socks5, it doesn't seem loading local shadow-socks configuration shall be the responsibility of TransportProvider

ios/MullvadREST/Transport/TransportStrategy.swift line 19 at r1 (raw file):

I put optional for the associated value in case .shadowsocks at TransportStrategy that nil means load shadow-socks proxy with local configuration.

ios/MullvadREST/Transport/TransportStrategy.swift line 45 at r1 (raw file):

I couldn't follow the second bullet.do you mean we need to save PersistentAccessMethod data type into UserDefaults instead of casting the string value ofUUID ?

ios/MullvadREST/Transport/TransportStrategy.swift line 86 at r1 (raw file):

fair enough.I doubt to get it done based on the discussion we had before on how to use extension

ios/MullvadRESTTests/TransportStrategyTests.swift line 10 at r1 (raw file):

done

ios/MullvadRESTTests/TransportStrategyTests.swift line 33 at r1 (raw file):

is not it a bit long? how about this :

Code snippet:

testUseDefaultStrategyWhenAllIsDisabled

ios/MullvadRESTTests/TransportStrategyTests.swift line 58 at r1 (raw file):

how about testReuseSameStrategyWhenEverythingElseIsDisabled

ios/MullvadRESTTests/TransportStrategyTests.swift line 83 at r1 (raw file):

ok

ios/MullvadRESTTests/TransportStrategyTests.swift line 152 at r1 (raw file):

Done.

ios/MullvadRESTTests/TransportStrategyTests.swift line 171 at r1 (raw file):

Indeed

Reviewed 2 of 6 files at r2, all commit messages.
Reviewable status: 2 of 19 files reviewed, 13 unresolved discussions (waiting on @buggmagnet and @mojganii)

ios/MullvadREST/Transport/TransportProvider.swift line 21 at r2 (raw file):

    private var currentTransport: RESTTransport?
    private let parallelRequestsMutex = NSLock()
    private var relayConstraints = RelayConstraints()

relayConstraints are not used anymore and can be removed.

ios/MullvadREST/Transport/TransportProvider.swift line 90 at r2 (raw file):

            do {
                let localConfiguration = try localShadowsocksLoader.configuration
                let config = configuration ?? localConfiguration

Nit: We don't need to try to load a local shadowsocks config unless incoming config is nil.

ios/MullvadREST/Transport/TransportStrategy.swift line 86 at r1 (raw file):

Doesn't really need to be an extension, just a public function.

ios/MullvadREST/Transport/TransportStrategy.swift line 131 at r2 (raw file):

        let fromData: (Data) -> UUID = { data in
            let str = String(data: data, encoding: .ascii)
            return UUID(uuidString: str!)!

Nit: These forced unwraps feel unsafe since incoming values are not guaranteed to be compatible. Sure, it's a private function, but I'm a little on the fence about it.

Reviewed 13 of 17 files at r1, 4 of 6 files at r2.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @mojganii)

ios/MullvadRESTTests/AccessMethodRepositoryStub.swift line 12 at r2 (raw file):

typealias PersistentAccessMethod = MullvadSettings.PersistentAccessMethod
class AccessMethodRepositoryStub: AccessMethodRepositoryDataSource {

Nit: Could this be a struct with a single let accessMethods: [[PersistentAccessMethod] instead?

ios/MullvadRESTTests/TransportStrategyTests.swift line 107 at r2 (raw file):

            transportStrategy.didFail()
        }
        XCTAssertEqual(transportStrategy.connectionTransport(), .direct)

This should be moved into the loop instead, comparing the last and current values to make sure they're not the same.

ios/MullvadSettings/AppStorage.swift line 38 at r2 (raw file):

}

protocol AnyOptional {

Nit: Do we need an extension for this when AppStorage is the only place it's used?

Reviewable status: 8 of 24 files reviewed, 15 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadREST/Transport/TransportProvider.swift line 21 at r2 (raw file):

Done.

ios/MullvadREST/Transport/TransportProvider.swift line 90 at r2 (raw file):

whenTransportStrategy fails to retrieve the configuration for bridges case,It iterates to the next one and preload new configuration for the next try attempt on bridges

ios/MullvadREST/Transport/TransportStrategy.swift line 86 at r1 (raw file):

the extension has been deleted

ios/MullvadREST/Transport/TransportStrategy.swift line 131 at r2 (raw file):

When we have default value for that and we are sure we're saving UUID it doesn't seem to me unsafe. after discussion Maroc, we came up to save string value od UUID like as before.

ios/MullvadRESTTests/AccessMethodRepositoryStub.swift line 12 at r2 (raw file):

Done

ios/MullvadRESTTests/TransportStrategyTests.swift line 107 at r2 (raw file):

Done.

ios/MullvadSettings/AppStorage.swift line 38 at r2 (raw file):

it's merged before,I've just moved it around.we can keep discussing out of the PR

Reviewed 2 of 12 files at r3, 7 of 13 files at r4, 7 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @buggmagnet and @mojganii)

ios/MullvadREST/Transport/AccessMethodIterator.swift line 19 at r5 (raw file):

    private var cancellables = Set<Combine.AnyCancellable>()

    /// `UserDefaults` key shared by both processes. Used to cache and synchronize last reachable api access method between them.

Nit: "Both processes", meaning packet tunnel and main app?

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 43 at r5 (raw file):

    public func preloadNewConfiguration() throws {
        // because the previous shadowsocks configuration was invalid, therefore generate a new one.

Nit: Why was it invalid?

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 55 at r5 (raw file):

        } catch {
            // There is no previous configuration either if this is the first time this code ran
            let newConfiguration = try create()

Nit: Reuse preloadNewConfiguration() instead?

Reviewed 7 of 17 files at r1, 2 of 12 files at r3, 7 of 13 files at r4, 7 of 7 files at r5.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadREST/Transport/AccessMethodIterator.swift line 19 at r5 (raw file):

Yes

ios/MullvadREST/Transport/AccessMethodIterator.swift line 61 at r5 (raw file):

    private func refreshCacheIfNeeded() {
        /// Validating the index of `lastReachableApiAccessCache` after any changes in `AccessMethodRepository`
        if let idx = enabledConfigurations.firstIndex(where: { $0.id == self.lastReachableApiAccessId }) {

We can afford not having to use abbreviations, let's rename this firstIndex

ios/MullvadREST/Transport/TransportStrategy.swift line 95 at r5 (raw file):

}

private extension PersistentProxyConfiguration.SocksConfiguration {

This should be defined in SocksConfiguration instead.

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 42 at r5 (raw file):

    }

    public func preloadNewConfiguration() throws {

We should rename this reloadConfiguration, and we can get rid of the comment, it doens't add information.

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 54 at r5 (raw file):

            return try shadowsocksCache.read()
        } catch {
            // There is no previous configuration either if this is the first time this code ran

Half of the comment is missin here, the original comment was

// There is no previous configuration either if this is the first time this code ran
// Or because the previous shadowsocks configuration was invalid, therefore generate a new one.

ios/MullvadRESTTests/AccessMethodIteratorTests.swift line 14 at r5 (raw file):

import XCTest

final class AccessMethodIteratorTests: XCTestCase {

It looks like this class is missing tests ?

ios/MullvadRESTTests/TransportStrategyTests.swift line 33 at r1 (raw file):

It's better to be explicit about test names since usually they test should be testing one thing only

ios/MullvadRESTTests/TransportStrategyTests.swift line 58 at r1 (raw file):

Sounds good

ios/MullvadRESTTests/TransportStrategyTests.swift line 159 at r5 (raw file):

    }
}

We are missing a couple of tests here

• A test to make sure that when the bridges configuration is suggested, but fails to load a shadowsocks configuration, the next available method will be suggested
• A test that verifies we do not enter an endless loop when only bridges API is selected, but we cannot load a shadowsocks configuration
• A test for a socks5 configuration (that the configuration returns the correct username / password combo)

ios/MullvadVPNTests/APIAccessMethodsTests.swift line 29 at r5 (raw file):

        }
    }

Given what we found, let's add a test to make sure that when settings are wiped, the 2 default access methods are still available

Reviewable status: 11 of 25 files reviewed, 15 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadREST/Transport/AccessMethodIterator.swift line 19 at r5 (raw file):

yep

ios/MullvadREST/Transport/AccessMethodIterator.swift line 61 at r5 (raw file):

Done.

ios/MullvadREST/Transport/TransportStrategy.swift line 95 at r5 (raw file):

Done.

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 42 at r5 (raw file):

Done.

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 43 at r5 (raw file):

when the relay is not available/reachable it should try with new configuration

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 54 at r5 (raw file):

Done.

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 55 at r5 (raw file):

reload just fetch and updates the cache

ios/MullvadRESTTests/TransportStrategyTests.swift line 33 at r1 (raw file):

Done.

ios/MullvadRESTTests/TransportStrategyTests.swift line 58 at r1 (raw file):

Done.

ios/MullvadRESTTests/TransportStrategyTests.swift line 159 at r5 (raw file):

Done.

ios/MullvadVPNTests/APIAccessMethodsTests.swift line 29 at r5 (raw file):

I'll do that but it's out of this PR

ios/MullvadRESTTests/AccessMethodIteratorTests.swift line 14 at r5 (raw file):

removed

Reviewed 1 of 17 files at r1, 13 of 14 files at r6, all commit messages.
Reviewable status: 24 of 25 files reviewed, 7 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadREST/Transport/TransportStrategy.swift line 82 at r6 (raw file):

        case let .socks5(configuration):
            switch configuration.authentication {
            case .noAuthentication:

nit
It's not a big deal, but ideally the TransportStrategy shouldn't care about how the SOCKS5 protocol works. (abstraction leaking)
The PersistentProxyConfiguration.usernamePassword should accept optional username and password instead, since the actual protocol negotiation happens in the SOCKS5 layer itself.

I will create a cleanup task to fix this.

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 37 at r6 (raw file):

    public func reloadConfiguration() throws {
        // because the previous shadowsocks configuration was invalid, therefore generate a new one.

This comment doesn't really make sense here anymore, we should remove it.

ios/MullvadRESTTests/ShadowsocksLoaderStub.swift line 31 at r6 (raw file):

    @discardableResult
    func load() throws -> ShadowsocksConfiguration {
        if hasError {

This implementation has a couple of drawbacks:

• You have to create the stub with an error by default, which doesn't make sense (a stub should not be in an error state by default, it should work immediately)
• You are forced to pass an error in the constructor even if you don't need it

instead of having a variable named hasError you can make the error optional and do it this way instead (not that we use the compiler to automatically generate the constructor for us here)

struct ShadowsocksLoaderStub: ShadowsocksLoaderProtocol {
    let configuration: ShadowsocksConfiguration
    let error: Error?

    func reloadConfiguration() throws {
        try load()
    }

    @discardableResult
    func load() throws -> ShadowsocksConfiguration {
        if let error { throw error }
        return configuration
    }
}

Also this is the pattern we've used in the tests so far (see OutgoingConnectionProxyStub)

ios/MullvadRESTTests/TransportStrategyTests.swift line 258 at r6 (raw file):

}

private enum IOError: Error {

nit
If we just need an error without any cases, we can use an empty struct instead like so

private struct IOError: Error { }

ios/MullvadSettings/AccessMethodRepository.swift line 96 at r6 (raw file):

    }

    public func reloadWithDefaultsAfterDataRemoval() {

nit
To be honest, I'm not feeling very happy with how we are doing this, but it works for now so we can leave it.

However, I want us to change this in the future. (I don't like the precedent it sets of re-adding things after we wiped all the settings)

Reviewable status: 24 of 25 files reviewed, 7 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadREST/Transport/TransportStrategy.swift line 82 at r6 (raw file):

The cleanup task is in IOS-455

Reviewed 14 of 14 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @mojganii)

ios/MullvadRESTTests/ShadowsocksLoaderStub.swift line 31 at r6 (raw file):

+1

ios/MullvadRESTTests/TransportStrategyTests.swift line 196 at r6 (raw file):

    }

    func testUsesSocks5WithAuthenticationWhenItReaches() throws {

Nit: Reaches, as in makes a successful connection?

ios/MullvadSettings/AccessMethodRepository.swift line 96 at r6 (raw file):

If we're simply adding the defaults again I think we can use add() instead of adding another function for it.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadSettings/AccessMethodRepository.swift line 96 at r6 (raw file):

I think what Mojgan did here is clearer from the API name instead of just calling add, the intent is stronger IMHO.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadREST/Transport/TransportStrategy.swift line 82 at r6 (raw file):

Good.

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 37 at r6 (raw file):

Done.

ios/MullvadRESTTests/ShadowsocksLoaderStub.swift line 31 at r6 (raw file):

Done.

ios/MullvadRESTTests/TransportStrategyTests.swift line 196 at r6 (raw file):

it means when the rotation reaches to Sock5

ios/MullvadRESTTests/TransportStrategyTests.swift line 258 at r6 (raw file):

the enum case makes more readable

ios/MullvadSettings/AccessMethodRepository.swift line 96 at r6 (raw file):

me either,Maybe we shouldn't remove the defaults in resetting process or returning/updating the defaults in fetchAll

Code snippet:

   public func fetchAll() -> [PersistentAccessMethod] {
        let accessMethods = (try? readApiAccessMethods()) ?? []
        if accessMethods.isEmpty {
            add([direct,bridge]) // or return defaults
        }
        return accessMethods
    }

Reviewed 2 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadSettings/AccessMethodRepository.swift line 96 at r6 (raw file):

This has been discussed offline

LGTM !

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii and @rablador)